feat(lapis): make the SILO info timeout configurable - #1866
feat(lapis): make the SILO info timeout configurable#1866corneliusroemer wants to merge 1 commit into
Conversation
callInfo() has had a hard-coded 100 ms request timeout since GenSpectrum#1593, picked so that the health endpoint cannot block when SILO does not respond. That same budget governs /sample/info, which deployments use as a readiness probe, so a SILO that is merely slow rather than down makes LAPIS report itself unhealthy and get taken out of service. Expose it as silo.infoTimeout, a Duration defaulting to 100ms, next to the existing silo.url. Nothing changes unless the property is set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PRvw1U8cSHWotT9kL5r8FC
|
@corneliusroemer-agent is attempting to deploy a commit to the cov-spectrum Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
🟡 Changes recommended
The new configurable timeout should be validated as positive to avoid runtime failures on misconfiguration, and the docs should clarify that it also impacts the SILO health check path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes the timeout used when LAPIS calls SILO’s /info endpoint configurable, keeping the existing 100ms behavior by default while allowing deployments to increase it to reduce flakiness when SILO is slow.
Changes:
- Inject a configurable
silo.infoTimeout(default100ms) intoCachedSiloClientand use it for the SILO info request timeout. - Document the new
--silo.infoTimeoutstartup parameter in the maintainer docs.
File summaries
| File | Description |
|---|---|
| lapis/src/main/kotlin/org/genspectrum/lapis/silo/SiloClient.kt | Replaces the hard-coded info request timeout with a configurable silo.infoTimeout value. |
| lapis-docs/src/content/docs/maintainer-docs/references/starting-silo-and-lapis.mdx | Adds documentation for the new --silo.infoTimeout parameter and its default. |
Review details
Suppressed comments (1)
lapis/src/main/kotlin/org/genspectrum/lapis/silo/SiloClient.kt:100
infoTimeoutis now configurable, butHttpRequest.Builder.timeout(...)rejects non-positive durations (throwsIllegalArgumentException). Without validation, a misconfiguration like0msor a negative value will crash the/infocall path at runtime; it’s better to fail fast during bean construction with a clear message.
@param:Value("\${silo.infoTimeout:100ms}") private val infoTimeout: Duration,
) {
private val httpClient = HttpClient.newBuilder()
// Create our own thread pool explicitly to not use the ForkJoinPool.commonPool()
// Use fixed pool with unbounded queue to prevent RejectedExecutionExeceptions
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| How long LAPIS waits when it asks SILO for its info (data version and SILO version), | ||
| e.g. `100ms`, `1s`. Defaults to `100ms`. |
f2965fb to
cf4902e
Compare
|
Superseded by #1868 - don't think it needs to be configurable, the timeout just needs to be restricted to where it's intended |
callInfo() has had a hard-coded 100 ms request timeout since #1593, picked so that the health endpoint cannot block when SILO does not respond. That same budget governs /sample/info, so a SILO that is merely a little slow (takes longer than 100ms) rather than down makes LAPIS return a 500. Users would not mind at all waiting say 200ms.
Rather than making a new judgment on what the timeout should be (there's a tradeoff between how long health can take if unhealthy and how slow silo can be before lapis returns 500) I propose to make the timeout configurable. So nothing changes unless users override.
Loculus would use this override to avoid flakes when SILO is just a bit slow (related to loculus-project/loculus#7298)
There's a separate issue about why info is sometimes slow: #682 - if that's still the case then 100ms is definitely too short and the default should be raised.
Claude-Session: https://claude.ai/code/session_01PRvw1U8cSHWotT9kL5r8FC